Skip to content

AtomicCounter refactor#21

Merged
msizov merged 14 commits into
mainfrom
feat/atomic-counter-manager
Jul 10, 2026
Merged

AtomicCounter refactor#21
msizov merged 14 commits into
mainfrom
feat/atomic-counter-manager

Conversation

@msizov

@msizov msizov commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Hot path for counter increments/retrival is lock-free, synchronization/flushes moved to separate manager routine

New CommitBatch method used for sync/flushes by manager routine

@msizov
msizov requested a review from a team as a code owner June 24, 2026 16:16
@msizov
msizov force-pushed the feat/atomic-counter-manager branch from ef575f0 to 364991e Compare July 1, 2026 11:20
Termina1 and others added 12 commits July 5, 2026 11:04
tla/ChotkiSync.tla models the replication protocol end to end: the
Syncer feed/drain state machines, pebble-snapshot diff sync with
sync-point batches applied on 'V', broadcast relaying over a replica
tree, and network read batching (record coalescing). TLC configs check
the fixed protocol (NoGaps, exact/complete block VVs, one diff sync per
src, quiescent convergence - all pass, see tla/README.md) and reproduce
three real protocol bugs as counterexamples:

1. Drain() did not rebroadcast a batch whose last record was 'B' (bye),
   nor the applied prefix of a batch failing mid-way. Records applied at
   the middle replica never reached downstream ones, and the next
   re-handshake merged the origin's version vector downstream without
   the data, making the loss permanent and silent. Drain now relays
   exactly the locally-applied prefix minus the session-scoped trailing
   'B' (Chotki.DrainApplied + Syncer.relayApplied).

2. The "allow only 1 diff sync per src" cleanup in drain() 'H' compared
   sync-point keys against cho.src, which never matches since a replica
   never drains its own handshake; stale sync points were never
   displaced. It now compares against the incoming handshake's source.

3. (found by TLC) A sync point outlived the session whose relayed 'H'
   created it: when a downstream link reconnected mid-relay, the relayed
   'D' records died with the old connection while the relayed 'V'
   arrived over the new one and applied the staged handshake VV without
   the data - permanent silent data loss on any network blip during a
   relayed diff sync. Sync points are now bound to the session that
   created them (Syncer.SessionId carried in the drain context) and
   aborted when it closes (Chotki.AbortSyncsVia).

Also fixed along the way (details in tla/README.md):
- draining a peer's bye no longer cuts our own diff short of its 'V'
  (the peer would silently miss the promised data; TestChotki_Sync3
  flaked on this)
- Syncer.Close raced concurrent Feed over the pebble snapshot iterators
  (mergingIter panic under -race); guarded by snapLock
- Feed's SendNone wait could block forever when a late Drain moved the
  drain state backwards after the one-shot unblock timer had fired
- WaitDrainState leaked two goroutines per call; processPings skipped
  the record following a removed ping; Feed's SendPing case mutated
  pingTimer without the lock
- ApplyD/ApplyV looped forever on truncated inner TLV records; ApplyV
  overwrote earlier errors; ApplyH merged a nil VV from a malformed
  relayed handshake; drain() 'H' registered the sync point even when
  ApplyH failed
- test fixes: examples left a DB open racing its cleanup; index tests
  waited only 5s for the background reindex worker
Each test fails against the pre-fix code:

- TestDrainRelaysAppliedRecordsWhenBatchEndsWithBye: a drained batch
  coalesced with the peer's 'B' (bye) must still be rebroadcast to the
  other sessions, minus the session-scoped trailing bye.
- TestHandshakeDisplacesStaleSyncPointOfSameSrc: a new handshake
  displaces the origin's older, unfinished sync point ("allow only 1
  diff sync per src").
- TestSyncerCloseAbortsItsSyncPoints: sync points die with the session
  that created them; a late 'V' gets ErrSyncUnknown instead of applying
  the staged handshake VV without the data.
- TestFeedFinishesDiffAfterPeerBye: a peer's bye must not cut our own
  diff short of its 'V' packet.
- TestApplyDVMalformedInputFailsFast: truncated inner TLV records fail
  fast instead of looping forever.
- TestProcessPingsFiltersAllPingsAndKeepsTheRest: ping filtering no
  longer skips the record following a removed ping.
- TestWaitDrainState*: no goroutine leaks for non-cancellable contexts
  or abandoned waits.
CommitPacket stamps new local ids from the cached cho.last under
commitMutex, while replication sessions can also advance cho.last when
they drain records stamped with our own src (our own history synced
back after a restore from an older snapshot) - holding only
cho.lock.RLock. The two paths share no lock, so a commit could observe
a stale cho.last and reuse an already issued seq: two different
mutations under one rdx.ID. rdx.ID is two uint64s, so a torn read was
possible on top of the lost update. MCBuggyLast.cfg is the TLA+
witness of the reuse; MCFixedLast.cfg models this fix.

Give the allocator cache its own mutex (lastLock) around the commit
allocation (nextLast), the own-source advance in drain, Last() and the
Close() reset. TestCommitAllocatorSyncedWithOwnSourceDrain races the
two paths and fails (under -race) against the pre-fix code.
@msizov
msizov force-pushed the feat/atomic-counter-manager branch from 364991e to 67f4900 Compare July 8, 2026 13:13
@msizov
msizov merged commit dd53e9a into main Jul 10, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants